-
Notifications
You must be signed in to change notification settings - Fork 4
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[+] Guzzle Transpost #68
base: master
Are you sure you want to change the base?
[+] Guzzle Transpost #68
Conversation
@@ -91,6 +97,7 @@ private function setOption(string $key, $value): void | |||
case 'integrationToken': | |||
case 'release': | |||
case 'url': | |||
case 'transport': |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd suggest to add a separate case for transport validation to restrict it only for allowed values
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@neSpecc Hawk\Transport::init
have validation
public static function init(Options $options)
{
$transports = self::getTransports();
if (!array_key_exists($options->getTransport(), $transports)) {
throw new TransportException('Invalid transport specified');
}
return new $transports[$options->getTransport()]($options);
}
src/Transport.php
Outdated
public static function init(Options $options) | ||
{ | ||
$transports = self::getTransports(); | ||
|
||
if (!array_key_exists($options->getTransport(), $transports)) { | ||
throw new TransportException('Invalid transport specified'); | ||
} | ||
|
||
return new $transports[$options->getTransport()]($options); | ||
} | ||
|
||
public static function getTransports(): array |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
docs missed
/** | ||
* @var string | ||
*/ | ||
protected $url; | ||
|
||
/** | ||
* @var int |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
docs missed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
docs are needed not only for typings. Add descriptions: why this property or function is used
|
} | ||
|
||
/** | ||
* @inheritDoc | ||
*/ | ||
public function send(Event $event): void | ||
protected function _send(Event $event): string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_send
method violates the PSR-12 recommendations.
It is not recommended to add underscores to method names.
*/ | ||
public static function getTransports(): array | ||
{ | ||
return [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using a hardcoded static registry in getTransports violates the Open-Closed Principle (OCP). Adding a new transport requires modifying the base class.
$response = $this->_send($event); | ||
|
||
try { | ||
$data = json_decode($response, true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$data = json_decode($response, true);
if (json_last_error() !== JSON_ERROR_NONE) {
...
}
* | ||
* @return mixed | ||
*/ | ||
public function send(Event $event) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
public function send(Event $event): ?array
Edits:
Hawk\Transport\GuzzleTransport
clientHawk\Transport\TransportInterface
transform toHawk\Transport
classurl
andtimeout
from child classesHawk\Transport::init
factoryHawk\Options::$transport
(curl
orguzzle
, defaultcurl
)Hawk\Exception\TransportException
default
andcustom
Hawk\Options
optionstimeout
,transport
How to pass new options:
PHPUnit results: